Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

New Quantity: Entropy #312

Merged
merged 4 commits into from
Nov 11, 2017
Merged

New Quantity: Entropy #312

merged 4 commits into from
Nov 11, 2017

Conversation

0xferit
Copy link
Contributor

@0xferit 0xferit commented Nov 7, 2017 •
edited by angularsen
Loading

Implemented Entropy quantity
Fixes #311

@angularsen
Copy link
Owner

angularsen commented Nov 7, 2017 •
edited
Loading

Just a tip, no need to add an issue first if you plan to create a pull request immediately anyway. The discussion can then continue in the PR instead. However, it may sometimes be good to create an issue first to discuss whether the feature is desired so you don't go through a lot of work only to get it rejected. Not that I plan to reject this, but just a heads up.

Also, please add "Fixes #000" to the PR description so that when the PR is merged it will auto-close that issue.

angularsen
angularsen requested changes Nov 7, 2017
View reviewed changes
Copy link
Owner

@angularsen angularsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding the other units also listed in this converter?
http://www.gordonengland.co.uk/conversion/entropy.htm

I also see there is something called Specific Entropy and Molar Entropy, just a heads up if you want to consider adding those too.

UnitsNet.Tests/CustomCode/EntropyTests.cs Outdated
{
public class EntropyTests : EntropyTestsBase
{
protected override double CaloriesPerKelvinInOneJoulePerKelvin => 1 / 4.184;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test constants should not be computations, but rather constants looked up in some reference material. This is an important step to avoid copying the same implementation over to the test. The values look correct though.

F.ex use this converter: http://www.gordonengland.co.uk/conversion/entropy.htm

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we should show computation steps in implementations, resulting values in tests, right?

For example: 1/4.184 in implementation, 0.239006 in test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can implement Specific Entropy. And you already merged Molar Entropy :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to work on other units you suggested.

Copy link
Owner

@angularsen angularsen Nov 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, the important part is to basically double check that the implementation is good. You can reuse the constant and multiple it by 1e3 for the various kilo/mega prefixes etc. That's fine I think and is easy to confirm.

@0xferit 0xferit changed the title Feature: Entropy [WIP] Feature: Entropy Nov 7, 2017
@0xferit
Copy link
Contributor Author

0xferit commented Nov 9, 2017

I see that some units has IT subscript, when I googled I saw that I 1 calorie equals to 4.1840 Joule and 1 calorieIT equals to 4.1868 Joule.

Also I haven't encountered to any calorieIT unit in the library yet.

Should I proceed and implement these (for example: calorieIT per degree Celsius)? Should I implement both calorie and calorieIT versions? What do you think?

@0xferit 0xferit changed the title [WIP] Feature: Entropy [WIP] New Quantity: Entropy Nov 9, 2017
@angularsen
Copy link
Owner

I have no domain knowledge here, so I can't say for sure whether both are widely used or not.
However, google's own unit converter does not include CalorieIT, and I don't find that many search results on the IT variants so that indicates to me it is perhaps not very widely used? I really don't know. If we are unsure if anyone will need it, then I vote to skip it until someone asks for it.

@0xferit
Copy link
Contributor Author

0xferit commented Nov 10, 2017

I agree that we should skip units with IT subscript for now, until someone asks for it. Then I think it's ready to merge, could you please review?

@0xferit 0xferit changed the title [WIP] New Quantity: Entropy New Quantity: Entropy Nov 10, 2017
angularsen
angularsen reviewed Nov 11, 2017
View reviewed changes
UnitsNet/UnitDefinitions/Entropy.json Outdated
{
"Name": "Entropy",
"BaseUnit": "JoulePerKelvin",
"XmlDoc": "",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you forget to add the xmldoc?

I'm adding in this text based on a few sections in the wiki:

Entropy is an important concept in the branch of science known as thermodynamics. The idea of "irreversibility" is central to the understanding of entropy. It is often said that entropy is an expression of the disorder, or randomness of a system, or of our lack of information about it. Entropy is an extensive property. It has the dimension of energy divided by temperature, which has a unit of joules per kelvin (J K−1) in the International System of Units.

@angularsen angularsen merged commit c1f83dd into angularsen:master Nov 11, 2017
angularsen added a commit that referenced this pull request Nov 11, 2017
@angularsen
Merge origin/master
dd904f6 
Squashed commit of the following:

commit 6e23232
Author: Andreas Gullberg Larsen <[email protected]>
Date:   Sat Nov 11 12:33:12 2017 +0100

    Fix headers

    Wrong year and use (c) instead of copyright symbol since git and powershell
    frequently messes up the encoding when using it.

commit c102e3e
Author: Andreas Gullberg Larsen <[email protected]>
Date:   Sat Nov 11 12:25:18 2017 +0100

    Regenerate code from PRs

commit c1f83dd
Author: Ferit Tunçer <[email protected]>
Date:   Sat Nov 11 13:55:29 2017 +0300

    Add quantity Entropy (#312)

commit 90d5f93
Author: Ferit Tunçer <[email protected]>
Date:   Sat Nov 11 13:43:12 2017 +0300

    Fix LapseRate Units (#321)

commit c94c1d2
Author: Ferit Tunçer <[email protected]>
Date:   Fri Nov 10 20:43:44 2017 +0300

    Delete duplicate quantity SubstanceAmount (#317)

commit 471d2fc
Author: Andreas Gullberg Larsen <[email protected]>
Date:   Thu Nov 9 21:25:20 2017 +0100

    Add Equals(T other, T maxError), obsolete Equals(T other) for Double quantities

    Equality comparison is not safe with System.Double as internal representation.
    Decimal quantities (Power, Information) still allow equality.
    Add test on new Equals() method.

commit e0eb3f0
Author: Andreas Gullberg Larsen <[email protected]>
Date:   Thu Nov 9 19:57:32 2017 +0100

    UnitsNet: 3.78.0

commit 041a53e
Author: Ferit Tunçer <[email protected]>
Date:   Thu Nov 9 21:54:51 2017 +0300

    Add quantity LapseRate (#316)

commit 8a4d648
Author: Andreas Gullberg Larsen <[email protected]>
Date:   Tue Nov 7 15:34:47 2017 +0100

    Fix email address

commit aa9a99a
Author: Ferit Tunçer <[email protected]>
Date:   Tue Nov 7 23:43:11 2017 +0300

    Add micropascalseconds (#314)

commit 0a585b8
Author: Ferit Tunçer <[email protected]>
Date:   Tue Nov 7 17:45:26 2017 +0300

    Add MolarEntropy (#310)

commit e51cd52
Author: Ferit Tunçer <[email protected]>
Date:   Tue Nov 7 17:04:30 2017 +0300

    Add MolarEnergy (#309)

commit 1fad2bb
Author: Ferit Tunçer <[email protected]>
Date:   Tue Nov 7 17:02:05 2017 +0300

    Add AmountOfSubstance (#304)

commit 6800561
Author: Ferit Tunçer <[email protected]>
Date:   Tue Nov 7 08:53:13 2017 +0300

    Add MolarMass Quantity Type (#305)

    * added MolarMass
    * added russian abbreviations

    NanogramPerMole to KilogramPerMole convertion tests were failing, so we set the tolerance to 1e-3 like in massTests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@angularsen angularsen angularsen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@0xferit @angularsen